-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gitpod support #4700
gitpod support #4700
Conversation
addCheck: false | ||
addComment: false | ||
addBadge: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the check version would be less intrusive here, if it correctly links to the same point. Looking at the docs, the badge doesn't look too bad though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addCheck would make CI fail if pre-building for gitpod fails
addComment would add a comment to every PR
addBadge adds a button on every PR, which doesn't send notifications
@@ -0,0 +1,30 @@ | |||
github: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like adding this new file to the root. In theory, gitpod should behave like bors and allow this file to live in the .github
folder.
I would assume it's an advertising feature for gitpod to not allow it to live in a non-root folder, but that's a scummy practise. See also gitpod-io/gitpod#729, which still doesn't suggest to use .github
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cart for putting a comment through.
I don't expect much movement though, unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cart for putting a comment through.
I don't expect much movement though, unfortunately.
cargo quickinstall wasm-bindgen-cli | ||
cargo quickinstall cargo-watch | ||
init: | | ||
cargo run -p build-wasm-example -- lighting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to pre-suppose which example is relevant. It would be very cool, for example, to select a relevant example which has been updated by the given PR. Or at least to have a dropdown with all the examples.
OTOH, this might be the best possible option within the technical constraints of the gitpod platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lighting example is one where a lot of things could go wrong, so it's a good first pick. My goal was mostly to cache everything for a wasm build, so why not go all the way to build an example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
select a relevant example which has been updated by the given PR. Or at least to have a dropdown with all the examples.
Thought: You could do this via a regular HTML form (hosted wherever), or a series of gitpod links hardcoded with an environment variable that triggers if/else logic in your configuration
See: https://www.gitpod.io/docs/environment-variables#providing-one-time-environment-variables-via-url
An example of a community doing this is drupal / drupalpod: https://github.com/shaal/DrupalPod
|
||
- name: Install linux dependencies | ||
before: | | ||
sudo apt-get install --no-install-recommends -yq libasound2-dev libudev-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're building for wasm, why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for rust-analyzer. I guess I could add config to have it work in wasm by default, but I'm not sure where that would live
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is rust-analyzer.cargo.target at wherever gitpod allow configuring rust-analyzer. For vscode that would be .vscode/settings.json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I added the file correctly, but still keeping the linux dependencies because they add almost nothing to the build time and they are still helpful
Testing this out I'm generally happy with the ease of use. Graphics and windowing aren't working though unfortunately, which limits the utility as both a reviewing and teaching tool. |
@alice-i-cecile you can run any examples that can be ran in wasm, you just need to build it with the new tool this PR adds. So you can also test graphics, with all the limitations wasm implies though. |
examples/README.md
Outdated
following command. | ||
|
||
```sh | ||
cargo run -p build-wasm-example -- lighting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a note somewhere near here that you can use this to change the default example that is being built for gitpod
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be honest I'm not completely sure how the gitpod integration will work out and would like it to be enabled before updating the "contributing" guide with this kind of info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any opinions on the actual configs, but it is really nice to be able to actually run code without having to pull the pr locally.
I tested it and I didn't experience any issues other than being a bit lost as to how this all works.
As IceSentry said, it's set up for building examples as wasm easily, and there's already a web server running to serve them. As for teaching, I'm working on another repo outside Bevy org for that, I think it would be nice with auto rebuild and auto reload of the wasm on every code change. Having that outside of the Bevy repo will make it a lot simpler with less code to be scared of. |
Since the gitpod part is controversial. Can you extract the wasm build tool to a separate PR since that part is not really controversial and makes building wasm simpler. |
Some assorted thoughts/questions:
|
ohwait it is running a webserver. Maybe it is a replacement for a build/run/test cycle 😄 |
Alright yeah this is pretty cool |
Yeah I'd merge this asap if it was split out. |
But yeah the biggest things holding me back at this point are:
I also agree with @DJMcNab that the extra dotfile at the root is regrettable. |
I've set up this repo: https://github.com/vleue/bevy_playground with the full experience:
From a change in the code to it being visible in the browser, it's around 4 seconds. I could probably reduce by a second by making cargo-watch and live.js check more often but that could be quite more heavy. I want to try writing a tutorial using that.
It is very similar. The two advantages of gitpod are prebuild and free tier Prebuild is a way to have it already build the environment and cache it for us. GitHub codespaces is closed, and paying from the start. Billing would be to Bevy, not split on each user. You can have similar result to the prebuild feature by using a custom docker image.
Yup I agree |
oh and most big Bevy contributors could be eligible to the open source free plan: https://www.gitpod.io/for/opensource |
done in #4776 |
# Objective - add an helper to build examples in wasm (from #4700) ## Solution - `cargo run -p build-wasm-example -- lighting`
Just submitted an application; I'll report back on how it goes. EDIT: approved; that was a painless process. |
# Objective - add an helper to build examples in wasm (from bevyengine#4700) ## Solution - `cargo run -p build-wasm-example -- lighting`
Sorry for letting this sit. I've been consolidating my thoughts over time: This seems like a useful workflow. It enables people to share around links to bevy branches and live-edit them. That being said, we shouldn't be in the business of supporting every useful workflow. This is why we don't check in things like If we adopt a particular workflow in the bevy repo (especially one visible from the root), it should be because we think it is "the one" workflow we use for a given part of our development process. The gitpod workflow is: "share a link to gitpod with a given repo/branch link, others open the link which logs them into their gitpod account and opens a browser hosted vscode instance backed by a vm, where changes are built for wasm and deployed to a server, which can be referenced via a link". So question number 1 is: what is this "the" workflow for?
Question number 2 is: do users wanting to use this workflow need us to check in a specific With "gitpod projects", users can manually provide a In theory we could set up a team for the bevy org and invite people in. But then we're sharing billing, which feels suboptimal. And it puts the burden on someone to manage team membership. Another alternative I looked into was gitpod's undocumented
Also note that while this might have a "@cart is pushing back on this" angle, I'm just trying to direct the conversation at this point / clarify our goals + priorities. Haven't made any decisions yet. |
# Objective - In #4966, @DJMcNab noted that the changes should likely have been flagged as controversial, and blocked on a final pass from @cart. - I think this is generally reasonable. - Added as an explicit guideline. - Changes to top-level files are also typically controversial, due to the high visible impact (see #4700 for a case of that). - Added as an explicit guideline. - The licensing information of our included assets is hard to find. - Call out the existence of CREDITS.md
# Objective - In bevyengine#4966, @DJMcNab noted that the changes should likely have been flagged as controversial, and blocked on a final pass from @cart. - I think this is generally reasonable. - Added as an explicit guideline. - Changes to top-level files are also typically controversial, due to the high visible impact (see bevyengine#4700 for a case of that). - Added as an explicit guideline. - The licensing information of our included assets is hard to find. - Call out the existence of CREDITS.md
# Objective - In bevyengine#4966, @DJMcNab noted that the changes should likely have been flagged as controversial, and blocked on a final pass from @cart. - I think this is generally reasonable. - Added as an explicit guideline. - Changes to top-level files are also typically controversial, due to the high visible impact (see bevyengine#4700 for a case of that). - Added as an explicit guideline. - The licensing information of our included assets is hard to find. - Call out the existence of CREDITS.md
# Objective - In bevyengine#4966, @DJMcNab noted that the changes should likely have been flagged as controversial, and blocked on a final pass from @cart. - I think this is generally reasonable. - Added as an explicit guideline. - Changes to top-level files are also typically controversial, due to the high visible impact (see bevyengine#4700 for a case of that). - Added as an explicit guideline. - The licensing information of our included assets is hard to find. - Call out the existence of CREDITS.md
# Objective - add an helper to build examples in wasm (from bevyengine#4700) ## Solution - `cargo run -p build-wasm-example -- lighting`
Objective
Solution
You can try this PR in gitpod at https://gitpod.io/#https://github.com/bevyengine/bevy/pull/4700